MINOR: Cleanup Trogdor Module #20214
Conversation
trogdor/src/main/java/org/apache/kafka/trogdor/coordinator/CoordinatorClient.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/workload/PayloadGeneratorTest.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/workload/PayloadGeneratorTest.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/workload/PayloadGeneratorTest.java
Outdated
Show resolved
Hide resolved
|
Thank you @m1a2st and @Pankraz76 for the reviews. I have addressed them. |
| @JsonCreator | ||
| public ErrorResponse(@JsonProperty("code") int code, | ||
| @JsonProperty("message") String message) { | ||
| this.code = code; | ||
| this.message = message; | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
nicely done, thanks. Until taken care by plugin it can happen anytime leaving this task kind of open. Offering to apply thins kind of conventions via rewrite.
|
Adding @chia7712 for review |
| @JsonCreator | ||
| public TaskRequest(@JsonProperty("taskId") String taskId) { | ||
| this.taskId = taskId == null ? "" : taskId; | ||
| } | ||
|
|
||
| @Override | ||
| @JsonProperty |
There was a problem hiding this comment.
the name taskId is same as the method name, so are those annotations really necessary?
There was a problem hiding this comment.
considering Override kind of best practice, it should not do any harm.
Can not tell anything about JsonProperty, except of assuming its needed by the business case, therefore covered by some test, or it can be removed/challenged.
There was a problem hiding this comment.
Hey @chia7712 , I agree, both of the annotations are unnexcessary. Dropped them.
Pls re-review. TIA!
There was a problem hiding this comment.
Thanks for the inspiration. False positive(s) should be covered as well.
There was a problem hiding this comment.
@sjhajharia thanks for this patch. I have left a couple of comments.
trogdor/src/main/java/org/apache/kafka/trogdor/agent/AgentClient.java
Outdated
Show resolved
Hide resolved
trogdor/src/main/java/org/apache/kafka/trogdor/basic/BasicNode.java
Outdated
Show resolved
Hide resolved
trogdor/src/main/java/org/apache/kafka/trogdor/coordinator/CoordinatorClient.java
Outdated
Show resolved
Hide resolved
trogdor/src/main/java/org/apache/kafka/trogdor/rest/TaskRequest.java
Outdated
Show resolved
Hide resolved
trogdor/src/main/java/org/apache/kafka/trogdor/rest/TasksRequest.java
Outdated
Show resolved
Hide resolved
| @@ -84,7 +83,9 @@ public List<Integer> partitionNumbers() { | |||
| } | |||
| return partitionNumbers; | |||
| } else { | |||
| return new ArrayList<>(partitionAssignments.keySet()); | |||
| ArrayList<Integer> partitionNumbers = new ArrayList<>(partitionAssignments.keySet()); | |||
| partitionNumbers.sort(Integer::compareTo); | |||
There was a problem hiding this comment.
This was done because TopicsSpecTest.testPartitionNumbers became flaky after the changes. Upon further consideration, I re-did the test rather than add a complexity here.
trogdor/src/main/java/org/apache/kafka/trogdor/workload/RandomComponent.java
Outdated
Show resolved
Hide resolved
trogdor/src/main/java/org/apache/kafka/trogdor/workload/TopicsSpec.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/common/MiniTrogdorCluster.java
Outdated
Show resolved
Hide resolved
trogdor/src/test/java/org/apache/kafka/trogdor/task/SampleTaskSpec.java
Outdated
Show resolved
Hide resolved
|
Thank you @chia7712 for the review. I have addressed the same. |
Now that Kafka support Java 17, this PR makes some changes in
trogdormodule. The changes mostly include:
Arrays.asList() are replaced with List.of()
with Map.of()
Some minor cleanups around use of enhanced switch blocks and conversion
of classes to record classes.
Reviewers: Ken Huang s7133700@gmail.com, Vincent Jiang
vpotucek@me.com, Chia-Ping Tsai chia7712@gmail.com